-
Notifications
You must be signed in to change notification settings - Fork 121
Log and track new events for Jetpack site performance improvements #16118
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Generated by 🚫 Danger |
|
|
RafaelKayumov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM. Scenarios work as described. Found just one issue. Pre-approving.
⚠️ TC1. I repeated the flow by logging out and logging in for several times without restarting the app. I can see numerous repetitive "Tracked" logs for the same event. I guess the observer/subscription was created but never cleared after a login or smth. Probably not related to the flow itself but to notification observer stacking.
🔵 Tracked jetpack_site_eligible_for_app_password_support, properties: []
🔵 Tracked jetpack_site_eligible_for_app_password_support, properties: []
🔵 Tracked jetpack_site_eligible_for_app_password_support, properties: []
🔵 Tracked jetpack_site_eligible_for_app_password_support, properties: []
🔵 Tracked jetpack_site_eligible_for_app_password_support, properties: []
🔵 Tracked jetpack_site_eligible_for_app_password_support, properties: []
🔵 Tracked jetpack_site_eligible_for_app_password_support, properties: []
🔵 Tracked jetpack_site_eligible_for_app_password_support, properties: []
🔵 Tracked jetpack_site_eligible_for_app_password_support, properties: [blog_id: <blogid> site_url: <siteurl>, plan: ecommerce-bundle, is_wpcom_store: true, was_ecommerce_trial: true]
- ✅ TC3
🔵 Tracked jetpack_site_flagged_unsupported_for_app_passwords, properties: [plan: jetpack_free, is_wpcom_store: false, store_id: <store id>, cause: major_error, api_error_code: application_passwords_disabled, flow: app_password_generation, http_status_code: 501, site_url: <site url>, blog_id: <blog id>, was_ecommerce_trial: false]
- ✅ TC4
🔵 Tracked jetpack_site_flagged_unsupported_for_app_passwords, properties: [http_status_code: 501, cause: major_error, flow: api_request, blog_id: <blog id>, api_error_code: incorrect_password, was_ecommerce_trial: false, store_id: <store id>, is_wpcom_store: false, site_url: <site url>, plan: jetpack_free]
- ✅ TC5
🔵 Tracked jetpack_site_flagged_unsupported_for_app_passwords, properties: [http_status_code: 500, store_id: <store id>, cause: general_failures_threshold_reached, is_wpcom_store: false, blog_id: <blog id>, site_url: <site url>, plan: jetpack_free, api_error_code: error, flow: api_request, was_ecommerce_trial: false]
- ✅ TC6
🔵 Tracked jetpack_site_flagged_unsupported_for_app_passwords, properties: [api_error_code: general_error, site_url: <site url>, is_wpcom_store: false, cause: general_failures_threshold_reached, was_ecommerce_trial: false, plan: jetpack_free, flow: app_password_generation, blog_id: <blog id>, store_id: <store id>, http_status_code: 500]
Good catch @RafaelKayumov! The issue was from the login flow where we authenticate a few times, causing the I moved the observers of notifications to |
RafaelKayumov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx! LGTM

Part of WOOMOB-1125
Description
This PR tracks new events for the network switching:
jetpack_site_eligible_for_app_password_support: tracked when a Jetpack site is detected as eligible to switch to application password authentication for network requests.jetpack_site_flagged_unsupported_for_app_passwordstracked when a site is flagged as not supported for app passwords. This comes with properties:flow:app_password_generationorapi_requestcause:major_errororgeneral_failures_threshold_reached.api_error_codehttp_status_codeTo add these events, the error handling needs to be updated to pass the errors for tracking:
RequestProcessor:AlamofireNetworkErrorHandler:DefaultStoresManager: clear app password locally to avoid the app picking up the password from a previous site upon switching sites, causing requests to fail with HTTP code 401 and errorincorrect_username.Testing steps
Use the test plan in pe5sF9-4Am-p2:
jetpack_site_eligible_for_app_password_supportis tracked after login (with an acceptable delay)jetpack_site_flagged_unsupported_for_app_passwordsis tracked with the following properties:flowcauseapi_error_codehttp_status_codeapp_password_generationmajor_errorapplication_passwords_disabledapi_requestmajor_errorincorrect_passwordapi_requestgeneral_failures_threshold_reachederrorapp_password_generationgeneral_failures_threshold_reachedgeneral_errorTesting information
Tested and confirm the above cases with simulator iPhone 16 iOS 18.2.
Screenshots
N/A
RELEASE-NOTES.txtif necessary.